Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove references of broadcastRawTransaction, use only broadcastTransaction #4900

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

edgarkhanzadian
Copy link
Contributor

@edgarkhanzadian edgarkhanzadian commented Feb 2, 2024

Try out this version of Leather — Extension build, Test report

Instead of using broadcastRawTransaction in one place and broadcastTransaction in another, it would be better to use only one function everywhere for consistency. Here's my take on refactoring that part.

Testing with the broadcastTransaction:

signingContract.mov

Copy link
Contributor

@fbwoolf fbwoolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @edgarkhanzadian, really great getting a new set of eyes on this code and seeing improvements. 👍

@kyranjamie
Copy link
Collaborator

This is great refactor, but a little concerned this changes behaviours of sponsors transactions, this logic isn't in the other broadcast fn right?

@fbwoolf
Copy link
Contributor

fbwoolf commented Feb 2, 2024

This is great refactor, but a little concerned this changes behaviours of sponsors transactions, this logic isn't in the other broadcast fn right?

Good point, thinking that can just be relocated?

@edgarkhanzadian
Copy link
Contributor Author

@fbwoolf @kyranjamie i tried replicating the sponsored functionality at 3e7af19 but i am not 100% sure that it works, how can i test it?

@fbwoolf
Copy link
Contributor

fbwoolf commented Feb 5, 2024

but i am not 100% sure that it works, how can i test it?

I believe there is a sponsored tx in the test-app to use?

@kyranjamie
Copy link
Collaborator

@fbwoolf @kyranjamie i tried replicating the sponsored functionality at 3e7af19 but i am not 100% sure that it works, how can i test it?

Alex swap uses sponsored txs right @fbwoolf ? Can't we use that to test?

@edgarkhanzadian
Copy link
Contributor Author

@kyranjamie i tried with a test app and it seems to be working for me, we can also try alex swaps

@edgarkhanzadian
Copy link
Contributor Author

Also, noticed an infinite loop on fee UI. Sponsored transactions shouldn't have any fees, but there is currently an infinite loop caused by the useEffects. Fixed

infiniteLoop.mov

@edgarkhanzadian
Copy link
Contributor Author

Merging. Please, don't include it in this release yet: #4918

@edgarkhanzadian edgarkhanzadian force-pushed the refactor/remove-broadcast-raw-transaction branch from c3a0354 to 0527317 Compare February 7, 2024 08:29
@edgarkhanzadian edgarkhanzadian force-pushed the refactor/remove-broadcast-raw-transaction branch from cc5985d to 2a6b0a0 Compare February 7, 2024 09:40
@edgarkhanzadian edgarkhanzadian added this pull request to the merge queue Feb 7, 2024
@edgarkhanzadian edgarkhanzadian removed this pull request from the merge queue due to a manual request Feb 7, 2024
@edgarkhanzadian edgarkhanzadian force-pushed the refactor/remove-broadcast-raw-transaction branch from 2a88579 to f5ec86c Compare February 7, 2024 09:56
@edgarkhanzadian edgarkhanzadian added this pull request to the merge queue Feb 7, 2024
Merged via the queue into dev with commit f0f488a Feb 7, 2024
28 checks passed
@edgarkhanzadian edgarkhanzadian deleted the refactor/remove-broadcast-raw-transaction branch February 7, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants